Add Gemma 3 Offline, Refactor API Key Flow, and Add Chat Scrollbar #48
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…scrollbar - Added Gemma 3n E4B it (offline GPU) model support using MediaPipe LlmInference. - Implemented ModelDownloadManager for downloading the 4.7 GB model file with storage checks. - Refactored API key management: removed forced popup on launch and implemented mandatory key checks during feature navigation. - Added a custom fading scrollbar indicator to the chat UI. - Updated project to use AGP 8.2.2 and Java 21 for MediaPipe compatibility. - Included updated signed APK: app-release-signed.apk.
Contributor
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review Summary
This PR introduces significant new functionality including offline Gemma 3 support, API key flow improvements, and UI enhancements. However, there are several critical issues that need to be addressed before merge:
Critical Issues Found:
- Resource Leak in ModelDownloadManager: HTTP connections and streams are not properly closed in finally blocks, which can cause resource exhaustion
- Logic Error in Offline Gemma: The implementation silently ignores image content, breaking multimodal functionality for offline models
- Crash Risk in LlmInference: Using non-null assertion operator without proper null safety can cause crashes
- Storage Validation Missing: Download dialog allows users to start downloads without sufficient storage space
Positive Changes:
- Good separation of concerns with the new ModelDownloadManager utility
- Proper API key validation flow improvements
- Clean integration of offline model support
- Appropriate build system updates for Java 21
Recommendations:
- Fix the resource management issues in ModelDownloadManager
- Add proper multimodal support validation for offline models
- Implement storage space validation before downloads
- Add null safety checks for LlmInference creation
The overall architecture and approach are sound, but these critical issues need resolution to ensure stability and proper functionality.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
Comment on lines
+44
to
+84
| try { | ||
| val url = URL(DOWNLOAD_URL) | ||
| val connection = url.openConnection() as HttpURLConnection | ||
| connection.connect() | ||
|
|
||
| if (connection.responseCode != HttpURLConnection.HTTP_OK) { | ||
| Log.e(TAG, "Server returned HTTP ${connection.responseCode}") | ||
| return@withContext false | ||
| } | ||
|
|
||
| val fileLength = connection.contentLengthLong | ||
| val input = connection.inputStream | ||
| val output = FileOutputStream(tempFile) | ||
|
|
||
| val data = ByteArray(1024 * 64) | ||
| var total: Long = 0 | ||
| var count: Int | ||
| while (input.read(data).also { count = it } != -1) { | ||
| total += count | ||
| if (fileLength > 0) { | ||
| onProgress(total.toFloat() / fileLength) | ||
| } | ||
| output.write(data, 0, count) | ||
| } | ||
|
|
||
| output.flush() | ||
| output.close() | ||
| input.close() | ||
|
|
||
| if (tempFile.renameTo(targetFile)) { | ||
| Log.i(TAG, "Model downloaded successfully to ${targetFile.absolutePath}") | ||
| true | ||
| } else { | ||
| Log.e(TAG, "Failed to rename temp file to target file") | ||
| false | ||
| } | ||
| } catch (e: Exception) { | ||
| Log.e(TAG, "Error downloading model", e) | ||
| if (tempFile.exists()) tempFile.delete() | ||
| false | ||
| } |
There was a problem hiding this comment.
🛑 Resource Leak: Connection resources are not properly closed in the finally block, which can cause resource leaks and network connection exhaustion.
Suggested change
| try { | |
| val url = URL(DOWNLOAD_URL) | |
| val connection = url.openConnection() as HttpURLConnection | |
| connection.connect() | |
| if (connection.responseCode != HttpURLConnection.HTTP_OK) { | |
| Log.e(TAG, "Server returned HTTP ${connection.responseCode}") | |
| return@withContext false | |
| } | |
| val fileLength = connection.contentLengthLong | |
| val input = connection.inputStream | |
| val output = FileOutputStream(tempFile) | |
| val data = ByteArray(1024 * 64) | |
| var total: Long = 0 | |
| var count: Int | |
| while (input.read(data).also { count = it } != -1) { | |
| total += count | |
| if (fileLength > 0) { | |
| onProgress(total.toFloat() / fileLength) | |
| } | |
| output.write(data, 0, count) | |
| } | |
| output.flush() | |
| output.close() | |
| input.close() | |
| if (tempFile.renameTo(targetFile)) { | |
| Log.i(TAG, "Model downloaded successfully to ${targetFile.absolutePath}") | |
| true | |
| } else { | |
| Log.e(TAG, "Failed to rename temp file to target file") | |
| false | |
| } | |
| } catch (e: Exception) { | |
| Log.e(TAG, "Error downloading model", e) | |
| if (tempFile.exists()) tempFile.delete() | |
| false | |
| } | |
| var connection: HttpURLConnection? = null | |
| var input: java.io.InputStream? = null | |
| var output: FileOutputStream? = null | |
| try { | |
| val url = URL(DOWNLOAD_URL) | |
| connection = url.openConnection() as HttpURLConnection | |
| connection.connect() | |
| if (connection.responseCode != HttpURLConnection.HTTP_OK) { | |
| Log.e(TAG, "Server returned HTTP ${connection.responseCode}") | |
| return@withContext false | |
| } | |
| val fileLength = connection.contentLengthLong | |
| input = connection.inputStream | |
| output = FileOutputStream(tempFile) | |
| val data = ByteArray(1024 * 64) | |
| var total: Long = 0 | |
| var count: Int | |
| while (input.read(data).also { count = it } != -1) { | |
| total += count | |
| if (fileLength > 0) { | |
| onProgress(total.toFloat() / fileLength) | |
| } | |
| output.write(data, 0, count) | |
| } | |
| output.flush() | |
| if (tempFile.renameTo(targetFile)) { | |
| Log.i(TAG, "Model downloaded successfully to ${targetFile.absolutePath}") | |
| true | |
| } else { | |
| Log.e(TAG, "Failed to rename temp file to target file") | |
| false | |
| } | |
| } catch (e: Exception) { | |
| Log.e(TAG, "Error downloading model", e) | |
| if (tempFile.exists()) tempFile.delete() | |
| false | |
| } finally { | |
| try { | |
| output?.close() | |
| input?.close() | |
| connection?.disconnect() | |
| } catch (e: Exception) { | |
| Log.w(TAG, "Error closing resources", e) | |
| } | |
| } |
Comment on lines
+759
to
+793
| private fun callOfflineGemma(context: Context, chatHistory: List<Content>, inputContent: Content): String { | ||
| val modelFile = com.google.ai.sample.util.ModelDownloadManager.getModelFile(context) | ||
| if (!modelFile.exists()) { | ||
| return "Error: Offline model not found at ${modelFile.absolutePath}" | ||
| } | ||
|
|
||
| // Initialize LlmInference if not already done | ||
| val llm = ScreenCaptureService.getLlmInferenceInstance(context) | ||
|
|
||
| // Build the prompt from chat history and input content | ||
| val promptBuilder = StringBuilder() | ||
|
|
||
| for (content in chatHistory) { | ||
| val role = if (content.role == "user") "user" else "model" | ||
| val text = content.parts.filterIsInstance<TextPart>().joinToString("\n") { it.text } | ||
| if (text.isNotBlank()) { | ||
| promptBuilder.append("<start_of_turn>$role\n$text<end_of_turn>\n") | ||
| } | ||
| } | ||
|
|
||
| val inputText = inputContent.parts.filterIsInstance<TextPart>().joinToString("\n") { it.text } | ||
| promptBuilder.append("<start_of_turn>user\n$inputText<end_of_turn>\n<start_of_turn>model\n") | ||
|
|
||
| val finalPrompt = promptBuilder.toString() | ||
| Log.d("ScreenCaptureService", "Offline Gemma Prompt: $finalPrompt") | ||
|
|
||
| return try { | ||
| val response = llm.generateResponse(finalPrompt) | ||
| Log.d("ScreenCaptureService", "Offline Gemma Response: $response") | ||
| response | ||
| } catch (e: Exception) { | ||
| Log.e("ScreenCaptureService", "Error in offline Gemma inference", e) | ||
| "Error: ${e.localizedMessage}" | ||
| } | ||
| } |
There was a problem hiding this comment.
🛑 Logic Error: The offline Gemma implementation ignores image content from both chat history and input content, only processing text parts. This breaks multimodal functionality for the offline model.
Suggested change
| private fun callOfflineGemma(context: Context, chatHistory: List<Content>, inputContent: Content): String { | |
| val modelFile = com.google.ai.sample.util.ModelDownloadManager.getModelFile(context) | |
| if (!modelFile.exists()) { | |
| return "Error: Offline model not found at ${modelFile.absolutePath}" | |
| } | |
| // Initialize LlmInference if not already done | |
| val llm = ScreenCaptureService.getLlmInferenceInstance(context) | |
| // Build the prompt from chat history and input content | |
| val promptBuilder = StringBuilder() | |
| for (content in chatHistory) { | |
| val role = if (content.role == "user") "user" else "model" | |
| val text = content.parts.filterIsInstance<TextPart>().joinToString("\n") { it.text } | |
| if (text.isNotBlank()) { | |
| promptBuilder.append("<start_of_turn>$role\n$text<end_of_turn>\n") | |
| } | |
| } | |
| val inputText = inputContent.parts.filterIsInstance<TextPart>().joinToString("\n") { it.text } | |
| promptBuilder.append("<start_of_turn>user\n$inputText<end_of_turn>\n<start_of_turn>model\n") | |
| val finalPrompt = promptBuilder.toString() | |
| Log.d("ScreenCaptureService", "Offline Gemma Prompt: $finalPrompt") | |
| return try { | |
| val response = llm.generateResponse(finalPrompt) | |
| Log.d("ScreenCaptureService", "Offline Gemma Response: $response") | |
| response | |
| } catch (e: Exception) { | |
| Log.e("ScreenCaptureService", "Error in offline Gemma inference", e) | |
| "Error: ${e.localizedMessage}" | |
| } | |
| } | |
| private fun callOfflineGemma(context: Context, chatHistory: List<Content>, inputContent: Content): String { | |
| val modelFile = com.google.ai.sample.util.ModelDownloadManager.getModelFile(context) | |
| if (!modelFile.exists()) { | |
| return "Error: Offline model not found at ${modelFile.absolutePath}" | |
| } | |
| // Check if input contains images - offline model may not support multimodal | |
| val hasImages = inputContent.parts.any { it is ImagePart } || | |
| chatHistory.any { content -> content.parts.any { it is ImagePart } } | |
| if (hasImages) { | |
| return "Error: Offline Gemma model does not support image processing. Please use an online model for multimodal queries." | |
| } | |
| // Initialize LlmInference if not already done | |
| val llm = ScreenCaptureService.getLlmInferenceInstance(context) | |
| // Build the prompt from chat history and input content | |
| val promptBuilder = StringBuilder() | |
| for (content in chatHistory) { | |
| val role = if (content.role == "user") "user" else "model" | |
| val text = content.parts.filterIsInstance<TextPart>().joinToString("\n") { it.text } | |
| if (text.isNotBlank()) { | |
| promptBuilder.append("<start_of_turn>$role\n$text<end_of_turn>\n") | |
| } | |
| } | |
| val inputText = inputContent.parts.filterIsInstance<TextPart>().joinToString("\n") { it.text } | |
| promptBuilder.append("<start_of_turn>user\n$inputText<end_of_turn>\n<start_of_turn>model\n") | |
| val finalPrompt = promptBuilder.toString() | |
| Log.d("ScreenCaptureService", "Offline Gemma Prompt: $finalPrompt") | |
| return try { | |
| val response = llm.generateResponse(finalPrompt) | |
| Log.d("ScreenCaptureService", "Offline Gemma Response: $response") | |
| response | |
| } catch (e: Exception) { | |
| Log.e("ScreenCaptureService", "Error in offline Gemma inference", e) | |
| "Error: ${e.localizedMessage}" | |
| } | |
| } |
Comment on lines
+796
to
+807
| fun ScreenCaptureService.Companion.getLlmInferenceInstance(context: Context): LlmInference { | ||
| val service = instance ?: throw IllegalStateException("ScreenCaptureService not running") | ||
| if (service.llmInference == null) { | ||
| val modelFile = com.google.ai.sample.util.ModelDownloadManager.getModelFile(context) | ||
| val options = LlmInference.LlmInferenceOptions.builder() | ||
| .setModelPath(modelFile.absolutePath) | ||
| .setMaxTokens(1024) | ||
| .build() | ||
| service.llmInference = LlmInference.createFromOptions(context, options) | ||
| } | ||
| return service.llmInference!! | ||
| } |
There was a problem hiding this comment.
🛑 Crash Risk: Using non-null assertion operator (!!) on llmInference without proper null safety can cause crashes if LlmInference.createFromOptions fails.
Suggested change
| fun ScreenCaptureService.Companion.getLlmInferenceInstance(context: Context): LlmInference { | |
| val service = instance ?: throw IllegalStateException("ScreenCaptureService not running") | |
| if (service.llmInference == null) { | |
| val modelFile = com.google.ai.sample.util.ModelDownloadManager.getModelFile(context) | |
| val options = LlmInference.LlmInferenceOptions.builder() | |
| .setModelPath(modelFile.absolutePath) | |
| .setMaxTokens(1024) | |
| .build() | |
| service.llmInference = LlmInference.createFromOptions(context, options) | |
| } | |
| return service.llmInference!! | |
| } | |
| fun ScreenCaptureService.Companion.getLlmInferenceInstance(context: Context): LlmInference { | |
| val service = instance ?: throw IllegalStateException("ScreenCaptureService not running") | |
| if (service.llmInference == null) { | |
| val modelFile = com.google.ai.sample.util.ModelDownloadManager.getModelFile(context) | |
| val options = LlmInference.LlmInferenceOptions.builder() | |
| .setModelPath(modelFile.absolutePath) | |
| .setMaxTokens(1024) | |
| .build() | |
| service.llmInference = LlmInference.createFromOptions(context, options) | |
| ?: throw RuntimeException("Failed to create LlmInference instance") | |
| } | |
| return service.llmInference!! | |
| } |
Comment on lines
+381
to
+399
| TextButton( | ||
| onClick = { | ||
| isDownloading = true | ||
| scope.launch { | ||
| val success = ModelDownloadManager.downloadModel(context) { progress -> | ||
| downloadProgress = progress | ||
| } | ||
| isDownloading = false | ||
| if (success) { | ||
| showDownloadDialog = false | ||
| Toast.makeText(context, "Download complete", Toast.LENGTH_SHORT).show() | ||
| } else { | ||
| Toast.makeText(context, "Download failed", Toast.LENGTH_LONG).show() | ||
| } | ||
| } | ||
| } | ||
| ) { | ||
| Text("OK") | ||
| } |
There was a problem hiding this comment.
🛑 Logic Error: The download dialog doesn't check available storage before allowing download. Users can start downloading a 4.7GB file even when they have insufficient storage, leading to failed downloads and wasted bandwidth.
Suggested change
| TextButton( | |
| onClick = { | |
| isDownloading = true | |
| scope.launch { | |
| val success = ModelDownloadManager.downloadModel(context) { progress -> | |
| downloadProgress = progress | |
| } | |
| isDownloading = false | |
| if (success) { | |
| showDownloadDialog = false | |
| Toast.makeText(context, "Download complete", Toast.LENGTH_SHORT).show() | |
| } else { | |
| Toast.makeText(context, "Download failed", Toast.LENGTH_LONG).show() | |
| } | |
| } | |
| } | |
| ) { | |
| Text("OK") | |
| } | |
| TextButton( | |
| onClick = { | |
| if (availableGB < 5.0) { | |
| Toast.makeText(context, "Insufficient storage space. At least 5 GB required.", Toast.LENGTH_LONG).show() | |
| return@TextButton | |
| } | |
| isDownloading = true | |
| scope.launch { | |
| val success = ModelDownloadManager.downloadModel(context) { progress -> | |
| downloadProgress = progress | |
| } | |
| isDownloading = false | |
| if (success) { | |
| showDownloadDialog = false | |
| Toast.makeText(context, "Download complete", Toast.LENGTH_SHORT).show() | |
| } else { | |
| Toast.makeText(context, "Download failed", Toast.LENGTH_LONG).show() | |
| } | |
| } | |
| } | |
| ) { | |
| Text("OK") | |
| } |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR introduces several major improvements to Screen Operator:
Gemma 3 Offline Support: Users can now select the "Gemma 3n E4B it (offline GPU)" model. If selected, a download manager ensures the 4.7 GB model file is present in the app's external files directory, showing available storage and a progress bar during download. Inference is handled locally via MediaPipe GenAI Tasks.
Refined API Key Flow: The initial mandatory API key popup has been removed to improve first-launch experience. Instead, the app checks for the required API key only when a user attempts to use an online model. The API key dialog is now dismissible, returning the user to the menu if they choose not to provide a key.
Chat Scroll Indicator: A subtle, semi-transparent gray scrollbar has been added to the chat interface. It appears on the right edge during scrolling and automatically fades out after 1 second of inactivity.
Build System Updates: The project has been upgraded to Android Gradle Plugin 8.2.2 and Java 21 to support the MediaPipe LLM Inference engine.
The signed release APK is included in the root directory as
app-release-signed.apk.PR created automatically by Jules for task 851222178467348793 started by @Android-PowerUser